Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Search Feature: Update map on prediction selection #192

Merged
merged 2 commits into from
Mar 3, 2018
Merged

Search Feature: Update map on prediction selection #192

merged 2 commits into from
Mar 3, 2018

Conversation

khusbuchandra
Copy link
Contributor

Integrate geolocation api and update map with a marker on the location based on user's selction for search prediction.

Issue: ##181

Issue Description:

User clicks on resulting options:
update map to location of selection and add marker at the location

Summary of solution:

  1. Integrated the geolocation api to get location details for selected prediction
  2. Update map to set view to the location
  3. Only one instance of marker on the map is shown now for search results.

Can this issue be closed?

No
To Do:

Have you named your branch in a descriptive way? Remember to name your branch in a unique and descriptive manner in order to properly reflect the issue or feature.

serach-result-selection

Thanks for contributing!

Integrate geolocation api and update map with a marker on the location based on user's selction for search prediction.
@khusbuchandra
Copy link
Contributor Author

Just figured, geolocation response doesn't have a name property which results to marker showing undefined for name in popup.

@@ -3,7 +3,7 @@ import style from './style';
import axios from 'axios';
import {API_SERVER} from '../../../config';
const OK_Status = 'OK';
Copy link
Contributor

@jkwening jkwening Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this to OK_STATUS so its clear that it's a true constant variable.

@@ -3,7 +3,7 @@ import style from './style';
import axios from 'axios';
import {API_SERVER} from '../../../config';
const OK_Status = 'OK';

let marker;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, let's make this a state variable of the search component instead of a global variable for the module. In the constructor, declare in the state object and initialize as null. I'll annotate the rest of the code where related changes are needed.

onPlaceFound(placeDetail) {
this.props.map.setZoom(16);
const location = placeDetail.geometry.location;
const userMarker = L.marker(location).addTo(this.props.map)

if (marker)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use this.state.marker in place of the marker variable throughout this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are managing the state in the parent,i.e, LeafletOSMMap, we then need to do it there? We can't change the state in the child right, only affect the changes through prop. I am new to React so, correct me if I am wrong or is there another correct pattern to do this?
Initially I intended to do all the marker stuff in parent component of Search so that I could affect the state, but based on the comment I figured we need to follow this approach.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redux is the most preferred way to handle store. Currently we don't use Redux in the project and redux makes me dizzy, the learning curve there with react feels too much for me to handle at present, that could be just me 😬 . I am comfortable with React's local store usage, in case we move to Redux I am back to reading.

Copy link
Contributor

@jkwening jkwening Mar 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redux would be new to me too but I don't think we need it in this case...will read up on it this weekend to confirm. But for this, each component can have its own state and consume props passed to it. See line 10 - we already have Search component specific state, add marker key to it for tracking Search result marker instance.

Basically, I'm considering this marker to be a local state of the Search component and its SearchResult child component - it should have complete control of the marker instance independent of it's parent. My thinking being because nothing other than the actions from Search(Result) determine the existence of the marker. Though this may change once we start integrating Places detail but for now I think this is the right way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok @jkwening will make changes to hold it in the search component state.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the dizzy 💫 feeling with redux is completely normal ... we don't need to go that deep just yet 🤔

if (marker)
this.props.map.removeLayer(marker);

marker = L.marker(location).addTo(this.props.map)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialize a new L.marker within this block:

this.setState({
  marker: <L.marker(location)...>
})

if (marker)
this.props.map.removeLayer(marker);

marker = L.marker(location).addTo(this.props.map)
.bindPopup(`<b>${placeDetail.name} </b>${placeDetail.formatted_address}`);
this.props.map.setView(location, 16);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should remain outside the this.setState() code snippet

@@ -59,29 +58,27 @@ export default class Search extends Component {
*
* @param {*} placeDetail
*/

onPlaceFound(placeDetail) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we should rename this function to "handleSelectedPlace" or some derivative of "handle..." since its handling the action of clicking on search prediction.

Modified Search & SearchResult component to incorporate the review comments.
@motosharpley
Copy link
Collaborator

Looks like you are doing a great job @khusbuchandra I am going to leave this for @jkwening to make the final call on merging because I am still playing catch up on all the updates you two have been implementing.

@jkwening jkwening merged commit e9ae23a into TheDevPath:development Mar 3, 2018
@jkwening jkwening removed the review label Mar 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants